feat(test suite): use cgroups to detect if a test leaks processes #6470

problame · 2024-01-25T12:43:54Z

Problem

Some tests leave stay processes behind after they exit.

This is the potential root cause for failed coverage-report generation, as well as other flakiness.

Solution

Before executing a test, create & enter a cgroup.
After the test is done, ensure that there are no processes left in the cgroup.

This banks on the assumption that the tests themselves don't do anything with the cgroup hierarchy, which is currently the case.

Changes

Use NeonEnvBuilder's __enter__ and __exit__ context manager hooks to implement the solution outlined above.
Fix the problems discovered using this mechanism:
- ```
fix(neon_local): leaks compute_ctl child process if get_status() fails`

Copy-pasting from #6474 here; as multiple TODO comments in this file
indicate, we should really be using background_process::start_process
```
- reorder vanilla_pg and neon_env_builder in one test where we saw failures
  => follow-up stray process check: move out of NeonEnvBuilder into an autouse fixture #6487
- fix test_neon_two_primary_endpoints_fail; it doesn't add to NeonEnv.endpoints,
  hence it didn't get stopped as part of NeonEnvBuilder.__exit__'s call to NeonEnv.stop.

Follow-Ups

Tested manually by commenting out NeonEnv.stop()'s self.attachment_service.stop() call.

test_runner/fixtures/neon_fixtures.py

koivunej

I think this is looking great!

koivunej · 2024-01-25T13:39:57Z

If we need to re-run until we see a coverage failure, this might be handy: fcf17a3

koivunej · 2024-01-25T15:28:33Z

Permission problems:

==================================== ERRORS ====================================
________ ERROR at teardown of test_pageserver_multiple_keys[debug-pg14] ________
[gw9] linux -- Python 3.9.2 /github/home/.cache/pypoetry/virtualenvs/neon-_pxWMzVK-py3.9/bin/python
/github/home/.cache/pypoetry/virtualenvs/neon-_pxWMzVK-py3.9/lib/python3.9/site-packages/allure_commons/_allure.py:221: in __call__
    return self._fixture_function(*args, **kwargs)
test_runner/fixtures/neon_fixtures.py:1383: in neon_env_builder
    yield builder
test_runner/fixtures/neon_fixtures.py:976: in __exit__
    with open(self.test_cgroup_dir.parent / "cgroup.procs", "a") as f:
E   PermissionError: [Errno 13] Permission denied: '/sys/fs/cgroup/neon_testsuite/cgroup.procs'

github-actions · 2024-01-25T18:55:29Z

No tests were run or test report is not available

Test coverage report is not available

_{The comment gets automatically updated with the latest test results
2978c83 at 2024-01-26T14:11:42.442Z :recycle:}

This reverts commit 4204a7d.

This reverts commit 194981c.

This reverts commit 407c78c.

koivunej · 2024-01-26T08:31:14Z

Looking great in https://neon-github-public-dev.s3.amazonaws.com/reports/pr-6470/7660119788/index.html#suites/90de3f9cafdc78be9db0b2ada81f7c26/ea7eab72b6180321:

2024-01-25 20:56:40.802 WARNING [neon_fixtures.py:994] SIGKILLing leaked process: 18949: ['/tmp/neon/bin/compute_ctl', '--http-port', '26120', '--pgdata', '/tmp/test_output/test_local_corruption[debug-pg14]/repo/endpoints/ep-3/pgdata', '--connstr', 'postgresql://cloud_admin@127.0.0.1:26119/postgres', '--spec-path', '/tmp/test_output/test_local_corruption[debug-pg14]/repo/endpoints/ep-3/spec.json', '--pgbin', '/tmp/neon/pg_install/v14/bin/postgres', '']

Was this an injected failure? No.

bayandin · 2024-01-26T08:41:38Z

Looking great in https://neon-github-public-dev.s3.amazonaws.com/reports/pr-6470/7660119788/index.html#suites/90de3f9cafdc78be9db0b2ada81f7c26/ea7eab72b6180321:

2024-01-25 20:56:40.802 WARNING [neon_fixtures.py:994] SIGKILLing leaked process: 18949: ['/tmp/neon/bin/compute_ctl', '--http-port', '26120', '--pgdata', '/tmp/test_output/test_local_corruption[debug-pg14]/repo/endpoints/ep-3/pgdata', '--connstr', 'postgresql://cloud_admin@127.0.0.1:26119/postgres', '--spec-path', '/tmp/test_output/test_local_corruption[debug-pg14]/repo/endpoints/ep-3/spec.json', '--pgbin', '/tmp/neon/pg_install/v14/bin/postgres', '']

Was this an injected failure? No.

It looks the similar thing as we discovered in #6270 (comment) — when endpoint start fails it leaves stray compute_ctl for some time

koivunej · 2024-01-26T08:57:35Z

If endpoint startup fails, probably we could just leave out the sync-safekeepers because no LSN should had changed, but perhaps that's a difficult call to make.. Alternatively that particular assert could be formulated as "basebackup fails".

Well, looking at control_plane/src/endpoint.rs this is quite clearly a problem of just not waiting for the spawned postgres to stop, similarly to #6474 but we probably cannot just kill it but let the sync safekeepers go through, as we cannot know that it was basebackup which failed.

problame · 2024-01-26T09:41:54Z

Dockerfile.buildtools

+# Add nonroot user
+RUN useradd -ms /bin/bash nonroot -b /home
+SHELL ["/bin/bash", "-c"]
+RUN echo "ALL   ALL = (ALL) NOPASSWD: ALL" >> /etc/sudoers


TOOD: security review

test_runner/fixtures/neon_fixtures.py

problame · 2024-01-26T09:55:45Z

.github/workflows/build_and_test.yml

@@ -420,19 +420,22 @@ jobs:
    container:
      image: 369495373322.dkr.ecr.eu-central-1.amazonaws.com/build-tools:${{ needs.build-buildtools-image.outputs.build-tools-tag }}
      # Default shared memory is 64mb
-      options: --init --shm-size=512mb
+      options: --init --shm-size=512mb --cgroupns=private --privileged


TODO security review

Asked for review in Slack https://neondb.slack.com/archives/C059ZC138NR/p1706265241134019

Copy-pasting from #6474 here; as multiple TODO comments in this file indicate, we should really be using background_process::start_process for compute_ctl => #6482

…lder-cgroup

This reverts commit 800d3d1.

areyou1or0 · 2024-01-26T10:59:13Z

Dropping the same comment I dropped on Slack:

But fundamentally, I think the --privileged is more risky, and I can't go without that (it's needed so the cgroup2fs mount is rw).

I suggest we implement an alternative - this increases privilege escalation risks majorly. It provides almost the same capabilities for the container as the host machine. There are so many ways to exploit this, implementing this would pose a big security threat. (via container escape, privilege escalation to cgroup manipulation, execute arbitrary commands, lateral movements etc. etc.)

grant passwdless sudo to the nonroot user

Like the above scenario, if an attacker gain access to the container, they can simply abuse the sudo privileges from the non-root user. This bypasses any security best practices.
You can provide sudo privileges to the specific commands only. We should definitely avoid sudo on nonroot user as this will basically make any non-root users run with root rights.
Instead of --privileged, why not use more fine-grained capabilities? Or perhaps use --mount on the necessary filesystems?

This change does NOT pass security review - please implement an alternative. @problame

areyou1or0

You can provide sudo privileges to the specific commands only. We should definitely avoid sudo on nonroot user as this will basically make any non-root users run with root rights.
Instead of --privileged, why not use more fine-grained capabilities? Or perhaps use --mount on the necessary filesystems?

.github/workflows/build_and_test.yml

problame · 2024-01-26T16:28:17Z

So, here's my hack to remount the cgroupfs in the container as rw and make delegate a cgroup subtree to our nonroot user.

Create a docker container.

cs@devvm-mbp:[~/src/neon-work-1/test]: docker run --rm -it --cgroupns=private test bash
nonroot@a2351caca14e:~$ echo $$
1

So, the bash process is PID 1 in the container's pid namespace.
What's its PID on the host?

cs@devvm-mbp:[~/src/neon-work-1/test]: docker inspect a2351caca14e
[
    {
        "Id": "a2351caca14eb2eb33ff08ff0be428012a252d0383c87262640e3fcaea1153f5",
        "Created": "2024-01-26T16:18:09.401859811Z",
        "Path": "bash",
        "Args": [],
        "State": {
            "Status": "running",
            "Running": true,
            "Paused": false,
            "Restarting": false,
            "OOMKilled": false,
            "Dead": false,
            "Pid": 2992050,
...

So, PID 1 in the container is PID 2992050 in the host PID namespace.

Enter that process's namespaces with our root privileges
remount the cgroupfs in the container as rw
create a cgroup neon_testsuite that's delegated to our nonroot user
- read up on delegated cgroups in section "Cgroups v2 delegation: nsdelegate and cgroup namespaces" of the man pages
move the container PID 1 into that cgroup
exit

cs@devvm-mbp:[~/src/neon-work-1/test]: sudo nsenter --target 2992050 --all
root@a2351caca14e:/# mount -o remount,rw /sys/fs/cgroup
root@a2351caca14e:/# mkdir /sys/fs/cgroup/neon_testsuite
root@a2351caca14e:/# chown nonroot:nonroot /sys/fs/cgroup/neon_testsuite
root@a2351caca14e:/# chown nonroot:nonroot /sys/fs/cgroup/neon_testsuite/cgroup.procs
root@a2351caca14e:/# echo 1 > /sys/fs/cgroup/neon_testsuite/cgroup.procs 
root@a2351caca14e:/# exit
logout

Now our nonroot user can operate on its delegated cgroup.

nonroot@a2351caca14e:~$ mkdir /sys/fs/cgroup/neon_testsuite/foo
nonroot@a2351caca14e:~$ mkdir /sys/fs/cgroup/neon_testsuite/foo^C
nonroot@a2351caca14e:~$ sleep 100000&
[1] 16
nonroot@a2351caca14e:~$ echo 16 > /sys/fs/cgroup/neon_testsuite/foo/cgroup.procs 
nonroot@a2351caca14e:~$

But it cannot operate on the root cgroup because it's owned by root

nonroot@a2351caca14e:~$ mkdir /sys/fs/cgroup/foo
mkdir: cannot create directory '/sys/fs/cgroup/foo': Permission denied
nonroot@a2351caca14e:~$ ls -lah /sys/fs/cgroup/
total 0
drwxr-xr-x 3 root    root    0 Jan 26 16:19 .
drwxr-xr-x 7 root    root    0 Jan 26 16:18 ..
-r--r--r-- 1 root    root    0 Jan 26 16:19 cgroup.controllers
-r--r--r-- 1 root    root    0 Jan 26 16:18 cgroup.events
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cgroup.freeze
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cgroup.max.depth
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cgroup.max.descendants
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cgroup.procs
-r--r--r-- 1 root    root    0 Jan 26 16:19 cgroup.stat
-rw-r--r-- 1 root    root    0 Jan 26 16:18 cgroup.subtree_control
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cgroup.threads
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cgroup.type
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpu.max
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpu.pressure
-r--r--r-- 1 root    root    0 Jan 26 16:19 cpu.stat
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpu.weight
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpu.weight.nice
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpuset.cpus
-r--r--r-- 1 root    root    0 Jan 26 16:19 cpuset.cpus.effective
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpuset.cpus.partition
-rw-r--r-- 1 root    root    0 Jan 26 16:19 cpuset.mems
-r--r--r-- 1 root    root    0 Jan 26 16:19 cpuset.mems.effective
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.1GB.current
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.1GB.events
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.1GB.events.local
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.1GB.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.1GB.rsvd.current
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.1GB.rsvd.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.2MB.current
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.2MB.events
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.2MB.events.local
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.2MB.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.2MB.rsvd.current
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.2MB.rsvd.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.32MB.current
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.32MB.events
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.32MB.events.local
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.32MB.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.32MB.rsvd.current
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.32MB.rsvd.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.64KB.current
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.64KB.events
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.64KB.events.local
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.64KB.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.64KB.rsvd.current
-rw-r--r-- 1 root    root    0 Jan 26 16:19 hugetlb.64KB.rsvd.max
-rw-r--r-- 1 root    root    0 Jan 26 16:19 io.max
-rw-r--r-- 1 root    root    0 Jan 26 16:19 io.pressure
-r--r--r-- 1 root    root    0 Jan 26 16:19 io.stat
-rw-r--r-- 1 root    root    0 Jan 26 16:19 io.weight
-r--r--r-- 1 root    root    0 Jan 26 16:19 memory.current
-r--r--r-- 1 root    root    0 Jan 26 16:18 memory.events
-r--r--r-- 1 root    root    0 Jan 26 16:19 memory.events.local
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.high
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.low
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.max
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.min
-r--r--r-- 1 root    root    0 Jan 26 16:19 memory.numa_stat
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.oom.group
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.pressure
-r--r--r-- 1 root    root    0 Jan 26 16:19 memory.stat
-r--r--r-- 1 root    root    0 Jan 26 16:19 memory.swap.current
-r--r--r-- 1 root    root    0 Jan 26 16:19 memory.swap.events
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.swap.high
-rw-r--r-- 1 root    root    0 Jan 26 16:19 memory.swap.max
drwxr-xr-x 3 nonroot nonroot 0 Jan 26 16:19 neon_testsuite
-r--r--r-- 1 root    root    0 Jan 26 16:19 pids.current
-r--r--r-- 1 root    root    0 Jan 26 16:19 pids.events
-rw-r--r-- 1 root    root    0 Jan 26 16:19 pids.max
-r--r--r-- 1 root    root    0 Jan 26 16:19 rdma.current
-rw-r--r-- 1 root    root    0 Jan 26 16:19 rdma.max

Epic: #6485 Before this PR, some tests would leak child processes. We found them using the approach in #6470. This PR fixes the findings because PR#6470 is being delayed due to security concerns.

problame · 2024-02-06T14:53:09Z

Paused until https://neondb.slack.com/archives/C059ZC138NR/p1707229781663129?thread_ts=1706265241.134019&cid=C059ZC138NR is resolved

problame changed the base branch from problame/iss-6366/refactor to main January 25, 2024 12:44

problame added 3 commits January 25, 2024 13:05

feat(test suite): use cgroups to detect if a test leaks processes

bcfd333

Tested manually by commenting out NeonEnv.stop()'s self.attachment_service.stop() call.

attempt to make it work in CI

b602063

guarantee leaked processes so we know this actually works

4204a7d

problame force-pushed the problame/neon-env-builder-cgroup branch from 9a4d27b to 4204a7d Compare January 25, 2024 13:05

problame requested review from bayandin and koivunej January 25, 2024 13:05

koivunej reviewed Jan 25, 2024

View reviewed changes

test_runner/fixtures/neon_fixtures.py Show resolved Hide resolved

koivunej approved these changes Jan 25, 2024

View reviewed changes

bayandin and others added 2 commits January 25, 2024 16:20

[DO NOT MERGE] build only debug build for v14

800d3d1

run tests sequentially

407c78c

problame added 6 commits January 25, 2024 18:59

neon_simple_env was not using test_cgroup_dir

415b489

[DO NOT MERGE] fail fast

194981c

see if this fixes the permission denied issue

23e36ae

Revert "guarantee leaked processes so we know this actually works"

bcb8bed

This reverts commit 4204a7d.

Revert "[DO NOT MERGE] fail fast"

193ba23

This reverts commit 194981c.

Revert "run tests sequentially"

b2ec54b

This reverts commit 407c78c.

bayandin marked this pull request as ready for review January 26, 2024 08:32

bayandin marked this pull request as draft January 26, 2024 08:33

problame mentioned this pull request Jan 26, 2024

fix(neon_local): slow init_tenant_mgr causes pageserver startup failure #6475

Closed

problame commented Jan 26, 2024

View reviewed changes

test_runner/fixtures/neon_fixtures.py Show resolved Hide resolved

problame commented Jan 26, 2024

View reviewed changes

fix(neon_local): leaks compute_ctl child process if get_status() fails

cedc037

Copy-pasting from #6474 here; as multiple TODO comments in this file indicate, we should really be using background_process::start_process for compute_ctl => #6482

problame added 2 commits January 26, 2024 10:09

add todo to protect against race condition with leaked threads

2b581ee

fix other detected process leakage in the lowest-effort-way possible

39490dd

This was referenced Jan 26, 2024

stray process check: move out of NeonEnvBuilder into an autouse fixture #6487

Open

test suite: eliminate bug class "stray processes after test exits" #6485

Open

stray process check: detect & protect against tests leaking Python threads #6486

Open

problame added 3 commits January 26, 2024 10:17

Merge remote-tracking branch 'origin/main' into problame/neon-env-bui…

a36be87

…lder-cgroup

Revert "[DO NOT MERGE] build only debug build for v14"

3c71003

This reverts commit 800d3d1.

fixup cedc037

1fdde9e

problame marked this pull request as ready for review January 26, 2024 10:26

problame requested review from a team as code owners January 26, 2024 10:26

problame requested review from save-buffer and removed request for a team and save-buffer January 26, 2024 10:26

areyou1or0 self-requested a review January 26, 2024 11:00

areyou1or0 requested changes Jan 26, 2024

View reviewed changes

avoid --privileged and blanket passwdless sudo

2978c83

koivunej reviewed Jan 26, 2024

View reviewed changes

.github/workflows/build_and_test.yml Show resolved Hide resolved

problame mentioned this pull request Jan 26, 2024

fix(test suite): some tests leak child processes #6497

Merged

problame self-assigned this Feb 6, 2024

bayandin removed their request for review June 4, 2024 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(test suite): use cgroups to detect if a test leaks processes #6470

feat(test suite): use cgroups to detect if a test leaks processes #6470

problame commented Jan 25, 2024 •

edited

Loading

koivunej left a comment

koivunej commented Jan 25, 2024

koivunej commented Jan 25, 2024

github-actions bot commented Jan 25, 2024 •

edited

Loading

koivunej commented Jan 26, 2024 •

edited

Loading

bayandin commented Jan 26, 2024

koivunej commented Jan 26, 2024

problame Jan 26, 2024

problame Jan 26, 2024

problame Jan 26, 2024

areyou1or0 commented Jan 26, 2024 •

edited

Loading

areyou1or0 left a comment

problame commented Jan 26, 2024 •

edited

Loading

problame commented Feb 6, 2024

feat(test suite): use cgroups to detect if a test leaks processes #6470

Are you sure you want to change the base?

feat(test suite): use cgroups to detect if a test leaks processes #6470

Conversation

problame commented Jan 25, 2024 • edited Loading

Problem

Solution

Changes

Follow-Ups

koivunej left a comment

Choose a reason for hiding this comment

koivunej commented Jan 25, 2024

koivunej commented Jan 25, 2024

github-actions bot commented Jan 25, 2024 • edited Loading

No tests were run or test report is not available

Test coverage report is not available

koivunej commented Jan 26, 2024 • edited Loading

bayandin commented Jan 26, 2024

koivunej commented Jan 26, 2024

problame Jan 26, 2024

Choose a reason for hiding this comment

problame Jan 26, 2024

Choose a reason for hiding this comment

problame Jan 26, 2024

Choose a reason for hiding this comment

areyou1or0 commented Jan 26, 2024 • edited Loading

areyou1or0 left a comment

Choose a reason for hiding this comment

problame commented Jan 26, 2024 • edited Loading

problame commented Feb 6, 2024

problame commented Jan 25, 2024 •

edited

Loading

github-actions bot commented Jan 25, 2024 •

edited

Loading

koivunej commented Jan 26, 2024 •

edited

Loading

areyou1or0 commented Jan 26, 2024 •

edited

Loading

problame commented Jan 26, 2024 •

edited

Loading